Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AGENT-950: Implement Separate JWT Tokens for Different User Personas #9039

Conversation

pawanpinjarkar
Copy link
Contributor

@pawanpinjarkar pawanpinjarkar commented Sep 18, 2024

  • Create 3 seperate JWT tokens- AGENT_AUTH_TOKEN, USER_AUTH_TOKEN, WATCHER_AUTH_TOKEN
  • Update the claim to set 'auth_scheme' to identify the user persona
  • Assisted service checks the auth_scheme to determine which user persona is allowed to access an endpoint
  • WATCHER_AUTH_TOKEN is used with header Watcher-Authorization and is used by wait-for command ( watcher persona)
  • USER_AUTH_TOKEN is used with header Authorization and is used by curl API requests, systemd services ( user persona)
  • AGENT_AUTH_TOKEN is used with header X-Secret-Key and is used by agent service ( agent persona)
  • Day2 monitor use case. Save WATCHER_AUTH_TOKEN in the secret
  • Rename var to a common name AUTH_TOKEN_EXPIRY. All the 3 tokens expire at the same time.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 18, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 18, 2024

@pawanpinjarkar: This pull request references AGENT-950 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

  • Create 3 seperate JWT tokens- AGENT_AUTH_TOKEN, USER_AUTH_TOKEN, WATCHER_AUTH_TOKEN
  • Update the claim to set 'sub' to identify the user persona
  • Pass different headers in the curl requests depending on the user persona, for example, Watcher-Authorization header for watcher user persona

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@pawanpinjarkar pawanpinjarkar marked this pull request as draft September 18, 2024 23:52
@pawanpinjarkar pawanpinjarkar marked this pull request as draft September 18, 2024 23:52
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 18, 2024
@pawanpinjarkar pawanpinjarkar force-pushed the create-seperate-tokens-4-each-user-authz branch from fbeb34c to 41f0f0c Compare September 19, 2024 00:05
@pawanpinjarkar pawanpinjarkar force-pushed the create-seperate-tokens-4-each-user-authz branch 2 times, most recently from 5d8f2b3 to e24c117 Compare October 2, 2024 15:19
@pawanpinjarkar pawanpinjarkar marked this pull request as ready for review October 2, 2024 15:19
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 2, 2024
@openshift-ci openshift-ci bot requested a review from rwsu October 2, 2024 15:20
@pawanpinjarkar
Copy link
Contributor Author

pawanpinjarkar commented Oct 3, 2024

/hold this depends on the assisted service PR openshift/assisted-service#6784

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 3, 2024
@pawanpinjarkar pawanpinjarkar force-pushed the create-seperate-tokens-4-each-user-authz branch 3 times, most recently from c6f2b0b to 3e2ce59 Compare October 7, 2024 21:05
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2024
@pawanpinjarkar pawanpinjarkar force-pushed the create-seperate-tokens-4-each-user-authz branch from 3e2ce59 to 5e0278d Compare November 5, 2024 17:18
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2024
@pawanpinjarkar pawanpinjarkar force-pushed the create-seperate-tokens-4-each-user-authz branch from 5e0278d to e67cde7 Compare November 5, 2024 18:08
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 5, 2024

@pawanpinjarkar: This pull request references AGENT-950 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

  • Create 3 seperate JWT tokens- AGENT_AUTH_TOKEN, USER_AUTH_TOKEN, WATCHER_AUTH_TOKEN
  • Update the claim to set 'auth_scheme' to identify the user persona
  • Pass different headers in the curl requests depending on the user persona, for example, Watcher-Authorization header for watcher user persona

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@pawanpinjarkar pawanpinjarkar force-pushed the create-seperate-tokens-4-each-user-authz branch from e67cde7 to 98721f8 Compare November 5, 2024 18:10
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 5, 2024

@pawanpinjarkar: This pull request references AGENT-950 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

  • Create 3 seperate JWT tokens- AGENT_AUTH_TOKEN, USER_AUTH_TOKEN, WATCHER_AUTH_TOKEN
  • Update the claim to set 'auth_scheme' to identify the user persona
  • Assisted service checks the auth_scheme to determine which user persona is allowed to access an endpoint
  • WATCHER_AUTH_TOKEN is used with header Watcher-Authorization and is used by wait-for command ( watcher persona)
  • USER_AUTH_TOKEN is used with header Authorization and is used by curl API requests, systemd services ( user persona)
  • AGENT_AUTH_TOKEN is used with header X-Secret-Key and is used by agent service ( agent persona)

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@pawanpinjarkar pawanpinjarkar force-pushed the create-seperate-tokens-4-each-user-authz branch from 98721f8 to 47ac905 Compare November 5, 2024 18:15
@@ -21,8 +22,8 @@ curl_assisted_service() {
;;
"GET")
curl -s -S -X GET "${additional_options[@]}" "${baseURL}${endpoint}" \
-H "Authorization: ${AGENT_AUTH_TOKEN}" \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PATCH doesn't need the ${authz} token?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes for PATCH got added here via rebase and originally came from a commit on Sep 30 i.e. faaddc1 however I think its a wrong commit because PATCH is not used in any curl requests in installer repo. The correct use is in appliance here and here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, for now the appliance isn't directly using the code (the func was copied to the codebase instead).
Will probably be handled later on to reuse this func, so I think it'd be safer to update the PATCH flow as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I will update the PATCH flow here(even if its unused now). Please note , later in the appliance, you will need to pass the USER_AUTH_TOKEN

func ParseExpirationFromToken(tokenString string) (time.Time, error) {
claims, err := ParseToken(tokenString)
if err != nil {
return time.Time{}, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something but I wonder why its necessary to return time on a failure since it seems to be unused on failure in gencrypto/authconfig.go. If its needed please add a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the comments

@bfournie
Copy link
Contributor

bfournie commented Nov 6, 2024

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2024
@pawanpinjarkar pawanpinjarkar force-pushed the create-seperate-tokens-4-each-user-authz branch from 9d0331b to e45f4d6 Compare November 13, 2024 03:57
@pawanpinjarkar
Copy link
Contributor Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2024
Copy link
Contributor

@rwsu rwsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@@ -3,26 +3,27 @@
curl_assisted_service() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a request fails because of an authorization error, it would be good to log the error so that it is visible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do it in a follow up PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, with the latest commit, the auth token can now be specified at only one place rather than sending it from n number of places when calling curl_assisted_service

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2024
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2024
@pawanpinjarkar
Copy link
Contributor Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 14, 2024
@pawanpinjarkar
Copy link
Contributor Author

/test integration-tests-nodejoiner

@bfournie
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2024
Copy link
Contributor

openshift-ci bot commented Nov 15, 2024

@pawanpinjarkar: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-agent-compact-ipv4-appliance-diskimage 4185056 link false /test e2e-agent-compact-ipv4-appliance-diskimage
ci/prow/okd-scos-e2e-aws-ovn 4185056 link false /test okd-scos-e2e-aws-ovn

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit 5e705f0 into openshift:master Nov 15, 2024
26 of 28 checks passed
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: ose-installer-altinfra
This PR has been included in build ose-installer-altinfra-container-v4.19.0-202411150809.p0.g5e705f0.assembly.stream.el9.
All builds following this will include this PR.

@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: ose-installer-terraform-providers
This PR has been included in build ose-installer-terraform-providers-container-v4.19.0-202411150809.p0.g5e705f0.assembly.stream.el9.
All builds following this will include this PR.

@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: ose-baremetal-installer
This PR has been included in build ose-baremetal-installer-container-v4.19.0-202411150809.p0.g5e705f0.assembly.stream.el9.
All builds following this will include this PR.

@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: ose-installer-artifacts
This PR has been included in build ose-installer-artifacts-container-v4.19.0-202411150809.p0.g5e705f0.assembly.stream.el9.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants